Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aarch64 CI test #829

Open
wants to merge 151 commits into
base: master
Choose a base branch
from
Open

aarch64 CI test #829

wants to merge 151 commits into from

Conversation

pfilipko1
Copy link
Collaborator

# the tests need the gprofiler image built (from Dockerfile). I run it separately here, because "docker build" prints the build logs
# more nicely. the tests will then be able to use the built image.
- name: Build gProfiler image
run: ./scripts/build_aarch64_container.sh -t gprofiler_aarch64 --output type=docker
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since #790, the gProfiler image is a blank image that contains the executable.

Please see the job build-container-x64 - it has

    needs:
      - build-executable-x64

to make sure the CI runs it after the exe is built, and then it uses the same binary exe built earlier to put it into the image, as done here:

      run: ./scripts/build_x86_64_container.sh --skip-exe-build --build-arg EXE_PATH=output/gprofiler_x86_64 -t gprofiler_x86_64

Please do the same here - by not passing --skip-exe-build you are rebuilding it unnecessarily, and plus, in any case, we'd like them to use the same file exactly.

with:
submodules: true

- name: Download the executable from previous job
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Download the image from previous job", no?

- name: Import gProfiler image
run: docker image load < output/gprofiler_aarch64.img

- name: Extract resources from gProfiler executable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you follow my suggestion from https://github.com/Granulate/gprofiler/pull/829/files#r1314022332 you can also simplify the code here to use the gProfiler executable instead of image - so it can run the exe and don't need the docker, the mappings etc, just like the x86_64 job.

- name: setup runner requirements
run: ./scripts/setup_runner_requirements.sh

# TODO: Add docker layer caching when GitHub Actions cache is stabilized and works good with "satackey/[email protected]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is not relevant here, remove pls

run: ./scripts/setup_runner_requirements.sh

# TODO: Add docker layer caching when GitHub Actions cache is stabilized and works good with "satackey/[email protected]"
- name: Run gProfiler tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The x86_64 tests run on 3.8, 3.9, 3.10.

I'm actually planning to deprecate 3.8 and 3.9 soon so there's no need to test all 3 - let's focus on 3.10.

Can you use the GH action to install python? i.e

    steps:
      - name: Set up Python 3.10
        uses: actions/setup-python@v2
        with:
          python-version: "3.10"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know what, let's actually do it with strategy like done in the x86_64 tests:

    strategy:
      matrix:
        python-version:
          - "3.10"

this is extendable for a future where we'll want to add 3.11, and also shows up nicely in the job name (gonna be "test-container-aarch64 (3.10)" like we have for the x86 ones)

Copy link
Contributor

@Jongy Jongy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave some comments.

This doesn't include test-executable-aarch64 but I'm okay with adding it later.

Note that CICD is failing - seems like a Docker issue where we're not using BuiltKit properly (BuildKit recognizes --platform and standard Docker doesn't iirc)


- name: Get and verify tag value
run: ./scripts/verify_tag.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need sudo?

@@ -8,7 +8,7 @@ set -euo pipefail
# Used in CI and checks that last pushed tag is greater than last existing tag.
# Using python package 'cmp_version' to do the compare work

pip install cmp_version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants